-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add typing for Directory Sync events #282
Conversation
workos/event_objects/directory.py
Outdated
class DirectoryType(Enum): | ||
AZURE_SCIM_v2 = "azure scim v2.0" | ||
BAMBOO_HR = "bamboohr" | ||
BREATHE_HR = "breathe hr" | ||
CEZANNE_HR = "cezanne hr" | ||
CYBERARK_SCIM_v2 = "cyperark scim v2.0" | ||
FOURTH_HR = "fourth hr" | ||
GENERIC_SCIM_v2 = "generic scim v2.0" | ||
GOOGLE = "gsuite directory" | ||
GUSTO = "gusto" | ||
HIBOB = "hibob" | ||
JUMPCLOUD_SCIM_v2 = "jump cloud scim v2.0" | ||
OKTA_SCIM_v2 = "okta scim v2.0" | ||
ONELOGIN_SCIM_v2 = "onelogin scim v2.0" | ||
PEOPLE_HR = "people hr" | ||
PERSONIO = "personio" | ||
PINGFEDERATE_SCIM_v2 = "pingfederate scim v2.0" | ||
RIPPLING_SCIM_v2 = "rippling v2.0" | ||
SFTP = "sftp" | ||
SFTP_WORKDAY = "sftp workday" | ||
WORKDAY = "workday" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need S3 or plain Rippling here, or are those totally deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah S3, Rippling (non-SCIM) and Gusto are completely deprecated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, so we can remove Gusto from this list also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to mode, these are the directory types that actually exist in produiction:
We don't actually have any Personio directories, which is no big deal.
But we do have Rippling directories (non-SCIM) in the DB. That's not reflected here.
We don't actually have any "untyped" directories in prod anymore, so we don't need to worry about this pending
state: https://github.com/workos/workos/blob/1b4c7a6a83b49ec3a605ff4f2f5dd17848e6085d/packages/api/src/directories/directories.controller.ts#L63
We should have a KTLO item to clean that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was for the events list so we wouldn't have events with these types anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm catching up to see we're doing API as well now lol
|
||
|
||
class DirectoryState(Enum): | ||
ACTIVE = "active" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we never show validating
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe there's ever an directory event emitted with status = validating
(or any status besides active, deleting).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where we exclude validating directories: https://github.com/workos/workos/blob/1b4c7a6a83b49ec3a605ff4f2f5dd17848e6085d/packages/api-services/src/directories/directories.service.ts#L322
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Via the directory API we definitely return other directory states, but the type is specifically event-scoped where we'd only have active or deleting directories. If we want to enumerate all states, we could extract this enum from the event context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, is the idea that we are only typing the events and not the dsync APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should type all the things, but I think that's worth a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. But I'm don't think we should release the SDK until both the Dsync events and API resources are typed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up adding some light typing for the APIs using existing objects. The list endpoints could use some better typing, but would require a size-able rework of the auto_paging_iter
to do so.
self.event: str = attributes["event"] | ||
self.id: str = attributes["id"] | ||
self.created_at = attributes["created_at"] | ||
self.data: DirectoryEvent = DirectoryEvent(attributes["data"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use this same type here. The directory deleted event is not the same shape as the directory activated event: https://workos.com/docs/events/directory-sync
It uses the directory type, not the legacy directory type with extra fields: https://github.com/workos/workos/blob/e95130bf78e01b202c7e7417adf36135cfc5f033/packages/api-json-models/src/events/directory-deleted.ts#L9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll update. Is DirectoryWithLegacyFields
still an accurate descriptor of the object we use for the other events, or is it here to stay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the events JSON models, it is accurate.
As for "here to stay", I think it contains fields that are no longer necessary. But I'm dubious about the dsync team prioritizing an effort to remove the unnecessary fields. So, likely here to stay for a while.
self.organization_id: str = attributes["organization_id"] | ||
self.created_at: str = attributes["created_at"] | ||
self.updated_at: str = attributes["updated_at"] | ||
self.raw_attributes: JsonDict = attributes.get("raw_attributes", {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PavanKulkarni didn't we stop emitting raw_attributes
for directory group events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me confirm this in LD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes only 3 teams still depend on the flag. That being said we still send raw_attributes
IIRC, it's just empty.
self.raw_attributes: JsonDict = attributes.get("raw_attributes", {}) | ||
self.created_at: str = attributes["created_at"] | ||
self.updated_at: str = attributes["updated_at"] | ||
self.role: Optional[DirectoryUserRole] = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually optional anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think so, but it's still optional in the API event schema, so will keep as-is until the API is updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the API resource that led me to ask this question. The role here always exists when returning a directory user: https://github.com/workos/workos/blob/e95130bf78e01b202c7e7417adf36135cfc5f033/packages/api/src/directory-users/directory-users.controller.ts#L340
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's optional! We don't return it on non-scim/Google users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ameesha but in the code I referenced role
always exists because getRoleFromUserRoleId
returns Promise<Role>
.
I see that the serializer accepts role?
, but in this code path I don't see how it can ever actually happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the role is always grabbed but in the serializer (line i sent), we only use it for scim/google dirs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing cares about whether the role is relevant in the directory type or not but the serializer. it's the only* thing that checks that logic (cause we create and set roles across the board).
`* there are other checks for directory type so we can end recalculations early but if we removed it, nothing would really change
@@ -78,4 +121,11 @@ def list_events( | |||
"method": Events.list_events, | |||
} | |||
|
|||
data = [] | |||
for list_data in response["data"]: | |||
if list_data["event"] in EVENT_TO_OBJECT.keys(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we add a new event or update an event type, until the SDK is explicitly updated we'll just ignore it
Description
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.